-
-
Notifications
You must be signed in to change notification settings - Fork 182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cpp to MPI type mapping improvements #3495
Conversation
Could you be more specific about which platforms the fixed width conversion fails? And what is the compile error? |
uint_8 is unsigned char on some platforms see e.g. https://en.cppreference.com/w/cpp/types/integer leading to the error with the redefinition |
Yes, just missed that one to revert :) The errors that you see now in the CI were the ones I discovered. Locally I couldn't reproduce it, but I also didn't check other versions.
|
C standard seems to only guarantee at least 64 bit width of However I couldn't find anything hinting a non 64-bit width on these platforms. |
For integers, to avoid any ambiguity I'd suggest providing interfaces only for fixed-width integers with the exception of |
Regarding the kahip interface, does anyone remember why we are we using unsigned long long there? Edit: I missed the link to the kahip source. Does kahip have a native C++ interface? Seems odd to have to go through a C interface. |
Don't think there is any other interface, at least it is the only occurrence of |
Couldn't get the |
This is the right approach if KaHiP is really using Looks good to me. @garth-wells you still had an open review comment about whether it's best just to use the MPI types directly in some cases. Could you comment further on that now? We should consider fixed width floats when we move to C++23 https://en.cppreference.com/w/cpp/types/floating-point |
https://github.com/KaHIP/KaHIP/blob/master/parallel/parallel_src/interface/parhip_interface.h#L15 |
mpi_type
is redesigned in favor of a type trait based system that is easier to extend. Especially this no longer requires a function call to obtain the mpi type, the interface change isdolfinx::MPI::mpi_type<T>() -> dolfinx::MPI::mpi_t<T>
Additionally the mapping is extended to previously non matched types.
Oddly the mapping of the fixed width
std
types to their MPI equivalent, for examplestd::int32_t -> MPI_INT32_T
seems to break on some platforms. No clue why, because using the standardC
type namings works, so are theMPI
types not pointing to the same ones?